Consider embedded properties in the QBE queries #2100
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
mp911de
added
type: enhancement
|
Signed-off the commits |
mipo256
force-pushed
the
GH-2099
branch
3 times, most recently
from
3d8fa6e to
a7f08ef
Compare
|
|
||
| Object actualPropertyValue = entityPropertiesAccessor.getProperty(property); | ||
|
|
||
| if (property.isEmbedded() && actualPropertyValue != null) { |
There was a problem hiding this comment.
Ideally, we don't distinguish based on whether the property is an embedded one but rather whether it is an entity. At some point it should be possible to express criteria based across objects that are part of the aggregate (like it is done in JPA, we consider embeddables and references).
.../main/java/org/springframework/data/relational/repository/query/RelationalExampleMapper.java
Outdated
Show resolved
Hide resolved
| * @return query | ||
| */ | ||
| private <T> Query getMappedExample(Example<T> example, RelationalPersistentEntity<?> entity) { | ||
| private <T> Query getMappedExample(Example<T> example, RelationalPersistentEntity<?> persistentEntity) { |
There was a problem hiding this comment.
What's the reason behind the renaming?
There was a problem hiding this comment.
Because one can confuse the PersistenceEntity with the entity as the actual instance/probe. I do not insist here at all, just my observation when working with code, what do you think, @mp911de ?
.../main/java/org/springframework/data/relational/repository/query/RelationalExampleMapper.java
Outdated
Show resolved
Hide resolved
| ) { | ||
| final List<Criteria> criteriaBasedOnProperties = new ArrayList<>(); | ||
|
|
||
| persistentEntity.doWithProperties((PropertyHandler<RelationalPersistentProperty>) property -> { |
There was a problem hiding this comment.
Nit: Use a for-loop instead of callback-style to reduce stack frames.
| * @param criteriaBasedOnProperties collection of {@link Criteria} objects to potentially enrich. | ||
| * @param currentPropertyPath the dot-separated path of the passed {@code property}. | ||
| */ | ||
| private void processEmbeddedRecursively( |
There was a problem hiding this comment.
Looks like a duplication of the general code that collects criteria for example matching. Should be consolidated.
…rties in QBE Signed-off-by: mipo256 <[email protected]> Signed-off-by: Михаил Поливаха <[email protected]>
|
@mp911de Just to make sure that we're on the same page. The current version of the algorithm accounts for the deeply nested embedded objects, like: The problem is that if we unroll the recursion into the for-loop, then the code may get a bit messy, since this is not a tail-recursive function we're dealing with. I've tinkered with it, and the only sensible approach I found is to flatten-out all the properties of all the embedded objects (included deeply embedded) on demand, and store them in some kind of I'm unsure that this is the best solution. From my personal, empirical data, I may humbly conclude that objects. Therefore the call stack will probably not be large at all. But again, this is merely my observation. Now, having said that, I can push the for-loop like approach with Queue and we'll have a look. WDYT, @mp911de, @schauder ? |
Closes #2099
Closes #1986
This PR enhances the
RelationalExampleMapperby making it consider embedded properties and specifiers for them.As it turned out during resolution of #1986, the embedded properties were kind of ignored in QBE.
Current algorithm works more a less the same way, it just traverses the
Examplerecursively from direct properties defined in the rootprobe, all the way down to the deeply embedded leafs.Tests are included in the PR.
CC: @mp911de @schauder